-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade Onfido native sdk to 7.4.0 + fix allowed documents #14802
Conversation
ac71a05
to
a80059d
Compare
a80059d
to
b3099fe
Compare
Adding screenshots for all platforms in the OP since @nkuoch is OOO. I'm running into an error on Android mWeb, I always seem to get this error even though I'm on the latest version of Chrome (110.0.5481.65): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build fails on Android:
BUILD FAILED in 5m 6s
error Failed to install the app. Make sure you have the Android development environment set up: https://reactnative.dev/docs/environment-setup.
Error: Command failed: ./gradlew app:installDebug -PreactNativeDevServerPort=8081
/Users/mariadcosta/Expensidev/App/android/app/src/debug/AndroidManifest.xml:11:7-34 Error:
Attribute application@supportsRtl value=(false) from AndroidManifest.xml:11:7-34
is also present at [com.onfido.sdk.capture:onfido-capture-sdk-core:15.4.0] AndroidManifest.xml:28:18-44 value=(true).
Suggestion: add 'tools:replace="android:supportsRtl"' to <application> element at AndroidManifest.xml:7:5-13:19 to override.
FAILURE: Build completed with 2 failures.
1: Task failed with an exception.
-----------
* What went wrong:
Execution failed for task ':app:processDebugMainManifest'.
> Manifest merger failed : Attribute application@supportsRtl value=(false) from AndroidManifest.xml:11:7-34
is also present at [com.onfido.sdk.capture:onfido-capture-sdk-core:15.4.0] AndroidManifest.xml:28:18-44 value=(true).
Suggestion: add 'tools:replace="android:supportsRtl"' to <application> element at AndroidManifest.xml:7:5-13:19 to override.
Looks like a conflict in the manifest.
Apply this diff worked but I'm not 100% if that's the correct approach:
diff --git a/android/app/src/main/AndroidManifest.xml b/android/app/src/main/AndroidManifest.xml
index 71c3dc6c9a..208076beeb 100644
--- a/android/app/src/main/AndroidManifest.xml
+++ b/android/app/src/main/AndroidManifest.xml
@@ -1,4 +1,5 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+ xmlns:tools="http://schemas.android.com/tools"
package="com.expensify.chat">
<uses-permission android:name="android.permission.INTERNET" />
@@ -15,7 +16,8 @@
android:roundIcon="@mipmap/ic_launcher_round"
android:allowBackup="false"
android:resizeableActivity="false"
- android:theme="@style/AppTheme">
+ android:theme="@style/AppTheme"
+ tools:replace="android:supportsRtl">
<activity
android:name=".MainActivity"
Also, Onfido on native Android does not allow selecting the document type, is that expected? Screen.Recording.2023-02-20.at.3.12.32.PM.movSame for iOS native: Screen.Recording.2023-02-20.at.3.37.58.PM.mov |
Got an error when running
Running |
b3099fe
to
278c1fe
Compare
Yes, native sdk doesn't allow to specify a list. You can upload any id, and Onfido will just guess what it is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The podfile check seems to be failing:
💥 lottie-react-native (5.1.4) not found in Podfile.lock. Did you forget to run `npx pod-install`?
@@ -1,4 +1,5 @@ | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android" | |||
xmlns:tools="http://schemas.android.com/tools" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cristipaval Would you mind checking these changes in this file? I was running into an error when building on android and these changes fixed it: #14802 (review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, this is fine. The error occurs because a library added as dependency to the App has android:supportsRtl="true"
in its AndroidManifext.xml
file. Or maybe the library doesn't have that property set at all, which means that the compiler sets the default value which is true
. This is in conflict with our App settings (android:supportsRtl="false"
) and the line added (tools:replace="android:supportsRtl"
) tells the compiler to set our App's value to that property to all libraries when there is a conflict. This is fine, because the setting disables support for languages which are written from right to left and we don't support those languages yet.
Updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! @Gonals Would you be able to fill out the reviewer checklist? The screenshots in the OP are mine so it would be good to get this tested by someone else.
Reviewer Checklist
Screenshots/VideosWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
Seems to work for me! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
🚀 Deployed to staging by https://github.com/nkuoch in version: 1.2.78-0 🚀
|
This performance regression appears to be consistent with what we were seeing on all PRs since last week, removing the DeployBlockerCash label |
🚀 Deployed to production by https://github.com/mountiny in version: 1.2.78-0 🚀
|
@@ -47,8 +47,8 @@ | |||
"@formatjs/intl-numberformat": "^6.2.5", | |||
"@formatjs/intl-pluralrules": "^4.0.13", | |||
"@gorhom/portal": "^1.0.14", | |||
"@oguzhnatly/react-native-image-manipulator": "github:Expensify/react-native-image-manipulator#5cdae3d4455b03a04c57f50be3863e2fe6c92c52", | |||
"@onfido/react-native-sdk": "7.0.1", | |||
"@oguzhnatly/react-native-image-manipulator": "github:Expensify/react-native-image-manipulator#c5f654fc9d0ad7cc5b89d50b34ecf8b0e3f4d050", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkuoch is on leave till June. I'm not sure if there was a specific reason, it might've been something we accidentally overlooked 🤔 Was the solution to the regression to update the hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MariaHCD yes, update hash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the author is on leave. We're hard to verify it. Based on the commit message, I think it's a accident:
278c1fe
Btw, with current package.json file, I can bump @oguzhnatly/react-native-image-manipulator to previous hash without any error.
Details
Fixed Issues
Part of https://github.com/Expensify/Expensify/issues/233363
Tests
Test on all platforms adding a verifying business bank account to a workspace on newDot, using random data.
Onfido flow should:
Make sure you can proceed until the bank account has been created (no matter the state)
Offline tests
N/A
QA Steps
Test on all platforms adding a verifying business bank account to a workspace on newDot, using random data.
Onfido flow should:
Make sure you can proceed until the bank account has been created (no matter the state)
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Screen.Recording.2023-02-20.at.3.20.21.PM.mov
Desktop
iOS
Screen.Recording.2023-02-20.at.3.37.58.PM.mov
Android
Screen.Recording.2023-02-20.at.3.12.32.PM.mov